Skip to content

fix: declare Hive partitioned scans as Range - #25279

Open
gstamatakis95 wants to merge 3 commits into
apache:mainfrom
gstamatakis95:issue-23436-hive-range-partitioning
Open

fix: declare Hive partitioned scans as Range#25279
gstamatakis95 wants to merge 3 commits into
apache:mainfrom
gstamatakis95:issue-23436-hive-range-partitioning

Conversation

@gstamatakis95

@gstamatakis95 gstamatakis95 commented Sep 13, 2026 •
edited
Loading

Copy link
Copy Markdown

Which issue does this PR close?

Rationale for this change

  • Hive grouped scans declare Partitioning::Hash, but no hash ever ran.
  • The optimizer treats any two Hash partitionings with equal counts as co-partitioned, so it skips the repartition.
  • Partitioned hash joins then drop rows. Fact {A, B, C} joined to dimension {A, C, D} returns only A, because fact group 1 holds B while dimension group 1 holds C.
  • Joining a Hive scan to a VALUES list also loses rows, since the planner hash repartitions the other side.

What changes are included in this PR?

  • Add FileGroup::group_by_partition_values_with_split_points, returning groups plus the SplitPoints between them.
  • Merge values into contiguous chunks rather than round robin. Only contiguous chunks can be described by split points. Group counts are unchanged.
  • Give ListFilesResult a partition_split_points field.
  • In ListingTable::scan build a logical Range partitioning from the partition columns and the split points and convert it with create_physical_partitioning, the same path a user declared output partitioning takes. No new public helper.
  • Declare nothing when split_file_groups_by_statistics re-cuts the groups. Comparing counts misses this, since the re-cut can return the same number.
  • Drop the dynamic filter guard in HashJoinExec since it existed only because Hive groups reported Hash.

What is the testing strategy for this PR?

  • TEST 14 and 15 in preserve_file_partitioning.slt cover the two failures above.
  • TEST 16 self joins the merged high cardinality table. TEST 17 asserts the statistics re-cut declares nothing.
  • Reverting only table.rs makes TEST 14 return one row instead of two and breaks TEST 12.
  • Six unit tests route every file through its split points and assert it lands in its own group, covering NULLs, compound keys, a NULL inside a split point, and unordered split points.
  • Two datafusion-proto round trips cover Dictionary(UInt16, Utf8) and Utf8View split points.

Are there any user-facing changes?

  • EXPLAIN prints Range([col ASC], [(v1), (v2)], N) instead of Hash([col], N).
  • Joins between Hive tables with different values gain a RepartitionExec(Range) rather than returning wrong results.
  • Partitioned dynamic filter pushdown works again under preserve_file_partitions.
  • File group membership changes when distinct values exceed target_partitions. Byte balance can shift on date partitions with larger recent days. Set preserve_file_partitions = 0 to restore the old behaviour.
  • API break: ListFilesResult gains a public field. Add partition_split_points: vec![] or use ...
  • Providers declaring Hash over value grouped files should declare Range from the split points instead. They already return missing rows today when the other join input is hash repartitioned.

@github-actions github-actions Bot added sqllogictest SQL Logic Tests (.slt) catalog Related to the catalog crate proto Related to proto crate datasource Changes to the datasource crate physical-plan Changes to the physical-plan crate labels Sep 13, 2026
@gstamatakis95 gstamatakis95 changed the title fix: declare Hive partitioned scans as Range instead of Hash partitio… fix: declare Hive partitioned scans as Range Sep 13, 2026
@gstamatakis95
gstamatakis95 marked this pull request as ready for review September 14, 2026 21:28
@stuhood stuhood mentioned this pull request Sep 17, 2026
Open
16 tasks
@stuhood

stuhood commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Thanks for opening this! I'll try to review soon.

One quick reference without having reviewed (sorry!): it's possible that there is some code reuse possible across the various TableProviders: see #22395 (comment)

stuhood
stuhood approved these changes Sep 21, 2026
View reviewed changes

@stuhood stuhood left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Comment thread datafusion/datasource/src/file_scan_config/mod.rs Outdated
Comment on lines 618 to 626
/// Builds output partitioning over `partition_cols` (resolved to their indices in
/// `schema`) with `partition_count` partitions. Returns `None` when there are no
/// partition columns. Callers use this to declare the output partitioning of a scan
/// whose file groups are organized by partition column values.
pub fn output_partitioning_from_partition_fields(
schema: &Schema,
partition_cols: &Fields,
partition_count: usize,
) -> Option<Partitioning> {

@stuhood stuhood Sep 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still returns Partioning::Hash by default, which means external consumers might still get the old behavior.

Should it be deprecated and replaced with a new function/method like?:

pub fn output_range_partitioning_from_split_points(
    schema: &Schema,
    partition_cols: &Fields,
    split_points: Vec<SplitPoint>,
) -> Result<Option<Partitioning>>;

...which could then be used in ListingTable::scan as well?

@gstamatakis95 gstamatakis95 Sep 22, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deprecated it in favor of output_range_partitioning_from_split_points with the signature you suggested and ListingTable::scan now uses it.

Comment thread datafusion/catalog-listing/src/table.rs Outdated
let ordering = table_partition_cols
.iter()
.map(|field| {
Expr::Column(Column::from_name(field.name())).sort(true, true)

@stuhood stuhood Sep 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If these bools are meant to correspond to SortOptions::default(), maybe they should be derived from it instead.

@gstamatakis95 gstamatakis95 Sep 22, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ordering now comes from PhysicalSortExpr::new_default, which is SortOptions::default(), the same default used to cut the groups.

@stuhood stuhood mentioned this pull request Sep 23, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@stuhood stuhood stuhood approved these changes
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

catalog Related to the catalog crate datasource Changes to the datasource crate physical-plan Changes to the physical-plan crate proto Related to proto crate sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Represent Hive/value partitioning without claiming Partitioning::Hash

2 participants

@gstamatakis95 @stuhood